[tcgc] add linter rulesets framework #1208
Conversation
iscai-msft
requested review from
lmazuel,
m-nash,
srnagar,
joheredi,
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
docs/libraries/typespec-client-generator-core/rules/require-client-suffix.md
Outdated
Show resolved
Hide resolved
…ypespec_azure_rulsets
|
All changed packages have been documented.
Show changes
|
| }, | ||
| }, | ||
| "incorrect-client-format": { | ||
| "invalid-client-format": { |
There was a problem hiding this comment.
it's used when we're checking the input of @clientFormat. Is this valid?
| @@ -929,7 +922,7 @@ export const $access: AccessDecorator = ( | |||
| ) => { | |||
| if (typeof value.value !== "string" || (value.value !== "public" && value.value !== "internal")) { | |||
| reportDiagnostic(context.program, { | |||
| code: "access", | |||
| code: "incorrect-access", | |||
There was a problem hiding this comment.
isn't it invalid-access in lib.ts that should make the build fail, doesn't it?
packages/typespec-client-generator-core/test/rules/require-client-suffix.test.ts
Outdated
Show resolved
Hide resolved
docs/libraries/typespec-client-generator-core/rules/require-client-suffix.md
Show resolved
Hide resolved
…ypespec_azure_rulsets
|
You can try these changes here
|
|
|
||
| | Name | Description | | ||
| | -------------------------------------------------------------------- | ----------------------------------------------------------------------- | | ||
| | `@azure-tools/typespec-client-generator-core/require-client-suffix` | Client names should end with 'Client'. | |
There was a problem hiding this comment.
can we specify which ruleset each of these are in?
There was a problem hiding this comment.
That file is auto generated with the list so would need to update the ref doc generator. If you have some idea on what'd you like to see feel free to suggest a new style but this should work for a generic linter
| 1. Write the rule in `typespec-azure/packages/typespec-client-generator-core/src/rules/[rule-name].rule.ts | ||
| 2. Add reference to the rule in the `rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter] | ||
| - This will automatically add it to a ruleset called `:all` for `@azure-tools/typespec-client-generator-core` | ||
| 3. Add the rule to the enable list for [`data-plane.ts`][data-plane-ruleset] and/or [`resource-manager.ts`][resource-manager-ruleset] in the [rulesets][rulesets] package. You can set `enable` to `false` here, if you want to delay enabling |
There was a problem hiding this comment.
Do we really want to have different rules for data plane and mgmt plane? I know we are less strict with mgmt plane, but aren't the recommendations the same for both?
There was a problem hiding this comment.
That's the current setup of azure-core-rulesets, where you explicitly list rules for the data plane and mgmt rulesets
There was a problem hiding this comment.
there is some rules that management plane specific and some that shouldn't apply to mamangement plane. But the majority should apply to both
There was a problem hiding this comment.
Do we have a single place to add a rule that applies to both or do we need to add the rule explicitly to both lists?
|
|
||
| **If you are adding a language-specific rule**, you will also need this extra step 4. Add reference to the rule in the `[language]Rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter] | ||
|
|
||
| For Azure generations then, all rules, including all language-specific rules, will be run on the specs. |
There was a problem hiding this comment.
I think we want to limit this to the Tier 1 languages not necessarily all languages? Should we specify this? I assume Go could add their own rules in here and those wouldn't apply unless generating Go even for azure?
There was a problem hiding this comment.
That's fair, let me update this to say that language-agnostic rules, and tier-1 languages will be run
| **If you are adding a language-specific rule**, you will also need this extra step 4. Add reference to the rule in the `[language]Rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter] | ||
|
|
||
| For Azure generations then, all rules, including all language-specific rules, will be run on the specs. | ||
| For unbranded generations, since we've added the rules into specific `best-practices:[language]` rulesets, you can explicitly specify a subset of rules in your `tsp-config.yaml`, i.e. if I only want Python best-practices, I could add this in my `tsp-config.yaml`: |
There was a problem hiding this comment.
This might create a bad situation where someone can turn off csharp rules but in reality they cannot. I think we might want to consider what flexibility a user would have over tcgc rules when running in azure context.
There was a problem hiding this comment.
every azure service is enforced to have one of the 2 rules set enabled by the spec repo pipeline.
There was a problem hiding this comment.
With rulesets, we have to give users the ability to turn them off because they are ultimately warnings. After talking with @timotheeguerin, you can't have error linter rules, only warnings. If there is a case where you explicitly want to error in csharp for a csharp-specific issue, the csharp emitter itself can throw a fatal error with reportDiagnostic, but the fatal error should have an analog in the linter warnings in tcgc that says "if you do this in csharp and ignore me, the csharp emitter will fail"
| Available ruleSets: | ||
|
|
||
| - `@azure-tools/typespec-client-generator-core/all` | ||
| - `@azure-tools/typespec-client-generator-core/best-practices:csharp` |
There was a problem hiding this comment.
What does best practice mean here? Does it mean this is optional? If so is there another category which is not optional?
There was a problem hiding this comment.
best practices is what I've called the ruleset for now, open to suggestions! I think the advice from the typespec team has been to use the word "best-practices", @timotheeguerin is this correct?
There was a problem hiding this comment.
Aren't these rulesets optional and can be opted out?
There was a problem hiding this comment.
oh I don't think we have a strong opinion on that name, we just have a placeholder @typespec/best-practices package that would contain misc linter to write good typespec.
I would say name this however makes the most sense. But I would note that I don't think this really should spend too much time on this as for all of azure they MUST use either the data-plane or resource-manager ruleset in teh spec repo. And I assume all those rules for all those language will be enabled.
I expect those more targeted ruleset might be more useful when we have the non azure version of tcgc(e.g. @typespec/client) where external customer might decide which language they care about.
|
|
||
| Available ruleSets: | ||
|
|
||
| - `@azure-tools/typespec-client-generator-core/all` |
There was a problem hiding this comment.
I am not sure all would ever be useful when considering non tier 1 languages as well as required rules vs best practices? Should we be a bit more granular on what all means?
There was a problem hiding this comment.
all isn't a specificly-defined ruleset, it's the standard name for all of the rules existing in a package
|
|
||
| Verify that there isn't a name conflict between a property in the model, and the name of the model itself | ||
|
|
||
| #### ❌ Incorrect |
There was a problem hiding this comment.
I think we should use Invalid over Incorrect.
There was a problem hiding this comment.
I'm following the current format which uses Incorrect over Invalid, @timotheeguerin should this be switched?
There was a problem hiding this comment.
Yeah just pattern from how eslint document its rules, e.g. https://typescript-eslint.io/rules/class-literal-property-style
But if we feel like changing then we can discuss the common format we want
| Available ruleSets: | ||
|
|
||
| - `@azure-tools/typespec-client-generator-core/all` | ||
| - `@azure-tools/typespec-client-generator-core/best-practices:csharp` |
There was a problem hiding this comment.
Aren't these rulesets optional and can be opted out?
| | Name | Description | | ||
| | -------------------------------------------------------------------- | ----------------------------------------------------------------------- | | ||
| | `@azure-tools/typespec-client-generator-core/require-client-suffix` | Client names should end with 'Client'. | | ||
| | `@azure-tools/typespec-client-generator-core/property-name-conflict` | Avoid naming conflicts between a property and a model of the same name. | |
There was a problem hiding this comment.
Is this a required rule or is it a recommendation to avoid property and model having the same name.
There was a problem hiding this comment.
all rules are in a sense optional its up to the user to enable them(directly or via a ruleset). For azure specs we require that the data-plane or resource-manager ruleset is used.
| In the `@azure-tools/typespec-client-generator-core` library, we have two main types of rules: | ||
|
|
||
| 1. Generic rule that applies to all emitted languages | ||
| 2. Specific rule that applies to a subset of all language: it can even apply to just one language |
There was a problem hiding this comment.
| 2. Specific rule that applies to a subset of all language: it can even apply to just one language | |
| 2. Specific rule that applies to a subset of all languages: it can even apply to just one language |
| 1. Write the rule in `typespec-azure/packages/typespec-client-generator-core/src/rules/[rule-name].rule.ts | ||
| 2. Add reference to the rule in the `rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter] | ||
| - This will automatically add it to a ruleset called `:all` for `@azure-tools/typespec-client-generator-core` | ||
| 3. Add the rule to the enable list for [`data-plane.ts`][data-plane-ruleset] and/or [`resource-manager.ts`][resource-manager-ruleset] in the [rulesets][rulesets] package. You can set `enable` to `false` here, if you want to delay enabling |
There was a problem hiding this comment.
Do we have a single place to add a rule that applies to both or do we need to add the rule explicitly to both lists?
|
|
||
| #### ✅ Ok | ||
|
|
||
| Using items from a private namespace within the same library is allowed. |
There was a problem hiding this comment.
This description doesn't seem to match the sample.
| [tcgc-linter]: https://github.com/typespec-azure/packages/typespec-client-generator-core/src/linter.ts "Linter TS File" | ||
| [rulesets]: https://github.com/typespec-azure/packages/typespec-azure-rulesets "Rulesets package" | ||
| [data-plane-ruleset]: https://github.com/typespec-azure/packages/typespec-azure-rulesets/src/rulesets/data-plane.ts "Data Plane Ruleset" | ||
| [resource-manager-ruleset]: https://github.com/typespec-azure/packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts "Resource Manager Ruleset" |
There was a problem hiding this comment.
These links don't resolve. They are missing /Azure/ after https://github.com and also blob/main/ after typespec-azure/
| 1. Write the rule in `typespec-azure/packages/typespec-client-generator-core/src/rules/[rule-name].rule.ts | ||
| 2. Add reference to the rule in the `rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter] | ||
| - This will automatically add it to a ruleset called `:all` for `@azure-tools/typespec-client-generator-core` | ||
| 3. Add the rule to the enable list for [`data-plane.ts`][data-plane-ruleset] and/or [`resource-manager.ts`][resource-manager-ruleset] in the [rulesets][rulesets] package. You can set `enable` to `false` here, if you want to delay enabling |
There was a problem hiding this comment.
I don't see step 3 done in this PR. Will this be added in a follow-up PR?
fixes #1147